Repository navigation
fix: remove the per-call struct format strings from the response parse path - #286
Conversation
|
Warning Review limit reachedNext included review available in 39 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
7f3b519 to
5e4c671
Compare
|
Rebased onto current Conflicts were only in Follow-up |
|
Tick the box to add this pull request to the merge queue (same as
|
…e path Commit 72d2aaf precompiled the request packers. That work covered the send path only. The receive path still built a format string on every call. get built '!L%ds' % (bodylen - 4). get_multi built '!L%ds%ds' % (keylen, bodylen - keylen - 4). CPython caches compiled struct formats in an LRU cache with 100 entries by default. A workload with more than 100 distinct value lengths evicts entries and recompiles on every call. A Struct for '!L%ds' is specific to one length, so precompiling one per length does not help. Both sites now read the fixed 4 byte flags field with a module-level struct.Struct('!L') and slice the rest. A slice compiles nothing. Measured on Python 3.12, parse step only, no socket: get, 500 distinct value lengths: 0.418 us -> 0.137 us (3.05x) get, 20 distinct value lengths: 0.227 us -> 0.129 us (1.77x) get, one repeated length: 0.227 us -> 0.121 us (1.87x) get_multi, 500 distinct value lengths: 0.495 us -> 0.187 us (2.65x) get_multi, one repeated length: 0.292 us -> 0.173 us (1.69x) The bytes() copy in _read_socket stays. The issue asked for a measurement and a decision. The copy is real. bytes(bytearray) costs 0.073 us at 64 bytes, 0.111 us at 1 KB, 1.80 us at 16 KB, and 17.4 us at 256 KB. Removing it changes public return types, because a bytearray slice is a bytearray: - deserialize returns the raw buffer for a value with the binary flag. get would return a bytearray in place of bytes. - get_multi uses the response key as a dict key. A bytearray is unhashable. - stats() tests "isinstance(key, bytes)" and would stop decoding the key. It would then use an unhashable bytearray as a dict key. - The error paths format extra_content into an exception message. The text changes from b'...' to bytearray(b'...'). Each of those needs its own bytes() call, which puts the copy back for the binary path and widens the change well past this issue. The acceptance criteria allow the copy to stay with a stated reason. This is the reason. Refs #276
Match main's ruff quote and slice style on FLAGS_UNPACKER. Co-authored-by: Jayson Reis <santosdosreis@gmail.com>
fcd5548 to
fd8178d
Compare
|
Commit type changed from |
What changed
bmemcached/protocol.pygains a module-levelFLAGS_UNPACKER = struct.Struct('!L').getandget_multiuse it to read the 4 byte flags field, then slice therest of the body. Neither builds a format string any more.
The
bytes()copy in_read_socketstays. The reason is below, with numbers.Why
Commit 72d2aaf precompiled the request packers, but that work covered the send
path only. The receive path still built a format string on every call:
CPython caches compiled struct formats in an LRU cache, 100 entries by
default. Because these formats embed the per-call value length, a workload
with more than 100 distinct value sizes evicts entries and recompiles on every
call. This is the same pattern 72d2aaf removed on the send side.
A
struct.Structfor'!L%ds' % nis specific to one lengthn, soprecompiling one per length does not help. A fixed-format read plus a slice
compiles nothing at all.
Verification
Result:
261 passed. This matches the baseline onmain.Result: 0 errors.
Benchmark
The benchmark runs the parse step only. No socket and no server are involved,
because the change is about format compilation. Python 3.12,
timeit.get, 500 distinct value lengths (past the LRU cache)get, 20 distinct value lengths (inside the cache)get, one repeated length (best case for the cache)get_multi, 500 distinct value lengthsget_multi, one repeated lengthThe gain is largest past the cache, as expected. It is still real inside the
cache, because a
%format and a lookup cost more than a slice.The
_read_socketcopy: measured, and keptThe issue asked for a measurement, not a theory. Here it is.
The copy is real.
bytes(bytearray)costs 0.073 us at 64 bytes, 0.111 us at1 KB, 1.80 us at 16 KB, and 17.4 us at 256 KB. At large value sizes it
dominates the parse step this pull request just made faster.
It still stays, because removing it changes public return types. A bytearray
slice is a bytearray, not bytes:
deserializereturns the raw buffer for a value carrying thebinaryflag.getwould return abytearrayin place ofbytes.get_multiuses the response key as a dict key. Abytearrayisunhashable, so this raises
TypeError.stats()testsisinstance(key, bytes)and would stop decoding the key. Itwould then use an unhashable
bytearrayas a dict key.extra_contentinto an exception message. The textchanges from
b'...'tobytearray(b'...').Each of those needs its own
bytes()call. That puts the copy back for thebinary path and widens the change well past this issue. The acceptance
criteria allow the copy to stay with a stated reason, so it stays.
The copy is worth its own issue, together with a decision about whether
returning
bytesfor binary values is a contract this project wants to keep.I have not opened that issue, because the answer is a design call for you, not
a defect. Say the word and I will open it.
Risks
A reviewer must check two points.
get, theold format read 4 bytes of flags then
bodylen - 4bytes of value, so thevalue is
extra_content[4:]. Forget_multi, the old format read 4 bytesof flags, then
keylenbytes of key, thenbodylen - keylen - 4bytes ofvalue, so the key is
extra_content[4:4 + keylen]and the value isextra_content[4 + keylen:].extra_contentis exactlybodylenbyteslong, which is what makes the open-ended slices correct.
_read_socketstill returnsbytes, soevery slice is
bytes, exactly asstruct.unpackproduced before. This isthe direct consequence of keeping the copy.
struct.unpackraisedstruct.erroron a body whose length did not match theheader. A slice does not. A short body now yields a short value in place of an
exception. No test covered that path, and the header check that #273 adds
catches the desynchronized-stream case that produces it.
Closes #276